-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core(tsc): fix audit details type hierarchy #7177
Conversation
wastedMs?: number; | ||
wastedBytes?: number; | ||
}; | ||
diagnostic?: Diagnostic; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
turns out I couldn't do [p: string]: Diagnostic
here because that means any string accessor of Table
should return a value assignable to Diagnostic
, which we don't want. So this is a middle ground, and seems flexible enough (@exterkamp rightly pointed out that if a Diagnostic
object can contain anything, why would you ever need two of them as properties on the same object?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
being able to define a set of properties with certain types and a different type for every other property not explicitly defined will be possible if/when negated types land in tsc (microsoft/TypeScript#29317)
totalBytes?: number; | ||
wastedMs?: number; | ||
diagnostic?: Diagnostic; | ||
[p: string]: number | boolean | string | undefined | Diagnostic; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otoh, any TableItem
or OpportunityItem
property can be a Diagnostic
because of how loose we allow the default type to be (almost but not quite any
)
|
||
// Contents of details below here | ||
// TODO(bckenny): unify Table/Opportunity headings and items on next breaking change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TableColumnHeading
and OpportunityColumnHeading
are very close but not close enough (e.g. label
vs text
). I think we just didn't get around to making them the same :S
Something for v5 :)
@@ -159,6 +159,7 @@ class BootupTime extends Audit { | |||
|
|||
const summary = {wastedMs: totalBootupTime}; | |||
|
|||
/** @type {LH.Audit.Details.Table['headings']} */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had to add a ton of these explicit headings
type lines because the heading itemType
now has a more stringent type (union of literals instead of just string
) which won't work if it's been widened to string
. There's an upcoming tsc 3.4 feature ("as const
") that may make this a bit less of an issue all over the codebase, but this doesn't seem too bad for now
const headings = [ | ||
{key: 'node', itemType: 'node', text: str_(UIStrings.failingElementsHeader)}, | ||
]; | ||
|
||
/** @type {LH.Audit.Details.Diagnostic|undefined} */ | ||
let diagnostic; | ||
if (impact || tags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left undefined if these weren't defined to match current LHR behavior
details: { | ||
type: 'diagnostic', | ||
// TODO: Consider not nesting diagnostics under `items`. | ||
items: [diagnostics], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awww this is just to remain non-breaking? :)
the idea would be details: {type: 'diagnostic', ...diagnostics}
if we were writing it today right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awww this is just to remain non-breaking?
Ha, well, since we just added this and we consider it internal only, I'm happy to change, I just didn't want to assume and/or cause churn in people's lives, especially because i mostly don't use these :)
We could change these newer ones and leave the older ones like metrics
until v5 since we know of at least a few people who rely on them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant "awwww" as in how thoughtful :D I'm probably the only person that relies on this and that would indeed be a pain, lol
I like leaving them until v5 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha, got it. SGTM :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I can actually understand the details type organization now! :D
@@ -11,6 +11,15 @@ | |||
|
|||
const Audit = require('./audit'); | |||
|
|||
/** @typedef { | |||
Partial<Record<LH.Artifacts.ManifestValueCheckID, boolean>> & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a long-term plan here with the multi-check audits?
I'm not sure the js typedefs are a readability win in this step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a long-term plan here with the multi-check audits?
I'm not sure the js typedefs are a readability win in this step.
ah, now that you say that, there's no reason it can't keep living in audit.d.ts
. I was deleting all the details
types from there, but there's no reason to consider it a details
type now, it's just a handy typedef. I'll put it back.
(personally I hate multi-check-audit and hope it dies as a base class someday, but that's more the long-long-term plan :)
@@ -160,8 +160,9 @@ class PerformanceCategoryRenderer extends CategoryRenderer { | |||
const thumbnailResult = thumbnailAudit && thumbnailAudit.result; | |||
if (thumbnailResult && thumbnailResult.details) { | |||
timelineEl.id = thumbnailResult.id; | |||
// @ts-ignore TODO(bckenny): fix detailsRenderer.render input type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input type? output type looks busted too based on the next line change 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input type? output type looks busted too based on the next line change
haha, maybe both then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this is great.
@@ -155,7 +101,7 @@ declare global { | |||
/** A more detailed description that describes why the audit is important and links to Lighthouse documentation on the audit; markdown links supported. */ | |||
description: string; | |||
// TODO(bckenny): define details | |||
details?: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ❤️ making details better!
Finally maps out types for the
details
objects!This PR was already getting long enough, so this (mostly) only touches the production of
details
object in audits. I'll follow up with a change to makedetails-renderer.js
and etc use these types. I already have that working in a branch, so I promise it will work on that side as well :)For the major thrust of things see
audit-details.d.ts
(and what was deleted fromaudit.d.ts
). Everything else is mostly just switching over to use the types, not changes to the audits themselves.The new
details
structure just describes what we currently do: there are a few details object shapes that we use directly (criticalrequestchain
,filmstrip
,opportunity
,screenshot
,table
, and nowdiagnostic
), then a bunch of things that are only ever used inside those details (text
,url
, 'code', etc). If and when we need some of thosecode
or whatever as top-level details, it's easy to add them to the union ofdetails
output by audits, just like it's easy to add additional properties.There are two relatively small changes to audit output:
no more
null
indetails
. This was a minor thing, but we only used it in two non-visible audits,as discussed in chat, a new
'diagnostic'
type. There are plenty of places you can still stick values that aren't visible in the final report indetails
(like unreferenced properties on tableitems
), but this is the place to stick full-on objects or entiredetails
objects likemetrics.js
ordiagnostics.js
. It's an explicit signal that a property won't be in the html report, and it's a way of keeping thedetails
types relatively narrow so that type checking is useful (you can still put anything in them, but they just have to be labeled withtype: 'diagnostic'
).In practice the effect of both are relatively minor (at most you get some
diagnostic
data one level deeper in the a11y audits). Check out the changes tosample_v2.json
for all the LHR changes that occur.